Review PR #15: allowed_origins via env var

📊 Score: 6/10

✅ Points positifs

1. Bonne intention - Rendre les CORS origins configurables est une excellente pratique. Hardcoder localhost:8030 limitait la flexibilité.

2. Intégration Pydantic Settings - L'utilisation de BaseSettings permet naturellement l'override via variable d'environnement ALLOWED_ORIGINS.

3. Code propre - L'implémentation est simple et suit le pattern existant du projet.

⚠️ Points à améliorer

1. Changement de port non documenté (8030 → 8080)

# Avant (main.py)
allow_origins=["http://localhost:8030"]

# Après (config.py)
allowed_origins: list[str] = ["http://localhost:8080"]

Pourquoi le changement de port ? Si c'est intentionnel (nouveau frontend sur 8080), documenter dans la description de la PR. Sinon, garder 8030 pour la rétrocompatibilité.

2. Pas de support CSV explicite pour ALLOWED_ORIGINS

Avec Pydantic Settings, pour setter plusieurs origins via env var:

# Actuellement (nécessite format JSON)
ALLOWED_ORIGINS='["http://localhost:8080", "https://example.com"]'

# Plus convivial avec un parser
ALLOWED_ORIGINS="http://localhost:8080,https://example.com"

Suggestion dans config.py:

from typing import Annotated
from pydantic import field_validator

class Settings(BaseSettings):
    allowed_origins: list[str] = ["http://localhost:8080"]
    
    @field_validator("allowed_origins", mode="before")
    @classmethod
    def parse_origins(cls, v):
        if isinstance(v, str):
            return [origin.strip() for origin in v.split(",")]
        return v

3. Typo dans le titre

"orgins" → "origins"

4. Pas de tests

Il manque un test pour valider que le CORS fonctionne avec différentes configurations d'origins.

💡 Suggestions

1. Corriger le port - Soit documenter le changement, soit revenir à 8030 (ou le port actuel du frontend)

2. Ajouter le validator CSV - Pour faciliter la configuration en production

3. Mise à jour de la doc - Ajouter ALLOWED_ORIGINS dans un README ou documentation de déploiement

4. Tests unitaires:

def test_cors_origins_from_env():
    with patch.dict(os.environ, {"ALLOWED_ORIGINS": "http://localhost:3000,https://app.example.com"}):
        settings = Settings()
        assert settings.allowed_origins == ["http://localhost:3000", "https://app.example.com"]

Verdict

PR fonctionnelle mais nécessite des ajustements avant merge. Le changement de port sans explication et l'absence de tests sont les principaux freins.

Actions requises:

• Clarifier le changement de port 8030 → 8080
• Corriger la typo dans le titre
• Ajouter tests CORS
• (Optionnel) Ajouter validator CSV pour plus de flexibilité